Skip to content

Prefer ID for hasTrait lookups#2562

Merged
kstich merged 1 commit intomainfrom
trait_lookup_opt
Mar 11, 2025
Merged

Prefer ID for hasTrait lookups#2562
kstich merged 1 commit intomainfrom
trait_lookup_opt

Conversation

@kstich
Copy link
Contributor

@kstich kstich commented Mar 11, 2025

This commit updates all calls to hasTrait from using Class based lookups to use ID based lookups. Traits are stored on a Shape in a ShapeId-keyed map. Using this will be faster than testing values via isInstance.

The lookup for hasTrait(ShapeId) is also updated to do a simple containsKey check on the map. This eliminates using findTrait which creates an intermediate Optional only to throw it away.

TraitLookups.hasTraitByClass       avgt    3  37.269 ± 2.546  us/op
TraitLookups.hasTraitByString      avgt    3  21.824 ± 1.066  us/op
TraitLookups.hasTraitByShapeIdOld  avgt    3  15.233 ± 5.002  us/op
TraitLookups.hasTraitByShapeId     avgt    3  11.611 ± 2.044  us/op

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kstich kstich requested a review from a team as a code owner March 11, 2025 06:02
@kstich kstich requested a review from joewyz March 11, 2025 06:02
}

boolean isIdempotent = shape.getTrait(IdempotentTrait.class).isPresent();
boolean isIdempotent = shape.hasTrait(IdempotentTrait.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
boolean isIdempotent = shape.hasTrait(IdempotentTrait.class);
boolean isIdempotent = shape.hasTrait(IdempotentTrait.ID);

@kstich kstich force-pushed the trait_lookup_opt branch from b2952ba to d6a31be Compare March 11, 2025 16:19
This commit updates all calls to `hasTrait` from using Class based
lookups to use ID based lookups. Traits are stored on a Shape in a
ShapeId-keyed map. Using this will be faster than testing values
via `isInstance`.

The lookup for `hasTrait(ShapeId)` is also updated to do a simple
`containsKey` check on the map. This eliminates using `findTrait`
which creates an intermediate `Optional` only to throw it away.
@kstich kstich force-pushed the trait_lookup_opt branch from d6a31be to 0dd9a6c Compare March 11, 2025 17:18
@kstich kstich merged commit 6ae0045 into main Mar 11, 2025
8 checks passed
@kstich kstich deleted the trait_lookup_opt branch March 11, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants